chore: Adding some testing on toProto conversion. #223
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @ehsavoie, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed!
This pull request focuses on improving the robustness and testability of Protobuf conversions within the spec-grpc module. The primary goal is to prevent NullPointerException issues that could arise when optional parameters are not explicitly set during conversion. To achieve this, a specific conversion method has been made null-safe, and comprehensive unit tests have been introduced using JUnit 5 to validate the correctness of various toProto conversion utilities. This ensures that the system handles missing optional data gracefully and that conversions behave as expected across different data structures.
Highlights
- Null-Safety for Protobuf Conversion: The 'message' conversion method in 'ProtoUtils' has been updated to prevent
NullPointerExceptionby ensuring thatcontextIdandtaskIdare set to empty strings if their original values are null. - Introduction of JUnit 5: New JUnit Jupiter dependencies have been added to the 'pom.xml', enabling the use of JUnit 5 for more robust and modern testing practices.
- Enhanced Test Coverage for Protobuf Conversions: A new test file, 'ToProtoTest.java', has been added, providing extensive unit test coverage for various
toProtoconversion methods, includingAgentCard,Task,Message, and different configuration/event types.
Using Gemini Code Assist
The full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips.
Invoking Gemini
You can request assistance from Gemini at any point in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
Customization
To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here.
You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Code Review
This pull request addresses a potential NullPointerException in the toProto conversion for Message objects by handling null contextId and taskId. It also introduces a comprehensive test suite for the ToProto utility class. My review focuses on improving the new tests for correctness and completeness, and suggests a minor code refinement for readability in the conversion logic. I've identified an issue with incorrect assertions in one of the new tests and a case where a field is silently dropped during conversion.
ehsavoie
force-pushed
the
grpc_optional
branch
2 times, most recently
from
8e62ad9 to
5fb6a26
Compare
ehsavoie
force-pushed
the
grpc_optional
branch
from
5fb6a26 to
bbd7208
Compare
| } | ||
| if (message.getTaskId() != null) { | ||
| builder.setTaskId(message.getTaskId()); | ||
| } |
There was a problem hiding this comment.
I think the corresponding message method in FromProto will need to be updated so the value is only set if non empty.
Checking that optional parameters not being set doesn't lead to a NullPointerException. Signed-off-by: Emmanuel Hugonnet <[email protected]>
ehsavoie
force-pushed
the
grpc_optional
branch
from
bbd7208 to
1e6b737
Compare
Checking that optional parameters not being set doesn't lead to a NullPointerException. Signed-off-by: Emmanuel Hugonnet <[email protected]>
2 participants
Checking that optional parameters not being set doesn't lead to a NullPointerException.